Skip to content

Bugfix/rdmp 359 internal catalogue bugs#2311

Merged
JFriel merged 9 commits intodevelopfrom
bugfix/RDMP-359-internal-catalogue-bugs
Feb 9, 2026
Merged

Bugfix/rdmp 359 internal catalogue bugs#2311
JFriel merged 9 commits intodevelopfrom
bugfix/RDMP-359-internal-catalogue-bugs

Conversation

@JFriel
Copy link
Collaborator

@JFriel JFriel commented Feb 9, 2026

Proposed Change

Prevent internal catalogues from beign drag-and-dropped into CICs
Prevent cohorts for being executed that have internal catalogues

Type of change

What types of changes does your code introduce? Tick all that apply.

  • Bugfix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation-Only Update
  • Other (if none of the other choices apply)

Checklist

By opening this PR, I confirm that I have:

  • Ensured that the PR branch is in sync with the target branch (i.e. it is automatically merge-able)
  • Created or updated any tests if relevant
  • Have validated this change against the Test Plan
  • Requested a review by one of the repository maintainers
  • Have written new documentation or updated existing documentation to detail any new or updated functionality and how to use it
  • Have added an entry into the changelog

Comment on lines +325 to +333
foreach (var cacac in cac.GetAggregateConfigurations())
{
if (cacac.Catalogue != null && cacac.Catalogue.IsInternalDataset)
{
task.CrashMessage = new ArgumentException($"Catalogue {cacac.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations.");
task.State = CompilationState.Crashed;
break;
}
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Where Note

This foreach loop
implicitly filters its target sequence
- consider filtering the sequence explicitly using '.Where(...)'.

Copilot Autofix

AI about 1 month ago

In general, to fix a "missed opportunity to use Where" in a foreach loop that immediately tests each element with an if and either continues or breaks, you replace the loop with a LINQ query that directly expresses the predicate, then act on the filtered result(s). This usually reduces nesting and makes it obvious you are searching or filtering, rather than performing arbitrary per-element logic.

For this specific case, the loop is only interested in whether there exists any aggregate configuration whose Catalogue is non-null and IsInternalDataset. On finding the first such configuration, it sets task.CrashMessage and task.State and stops iterating. The clearest equivalent using LINQ is to obtain the first offending configuration via FirstOrDefault, then if that result is non-null, set CrashMessage/State. We already have using System.Linq; at the top, so no new imports are needed. The behavior remains unchanged: there is at most one place where CrashMessage/State are set in this block, and enumeration stops as soon as the first problematic configuration is found.

Concretely, in Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs, replace the foreach block at lines 325–333 with code that calls cac.GetAggregateConfigurations().FirstOrDefault(...) using the same predicate (ac => ac.Catalogue != null && ac.Catalogue.IsInternalDataset). If a matching configuration is found, construct the same ArgumentException using that configuration’s Catalogue.Name and set task.State = CompilationState.Crashed;. No additional methods or fields are required.

Suggested changeset 1
Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs b/Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs
--- a/Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs
+++ b/Rdmp.Core/CohortCreation/Execution/CohortCompiler.cs
@@ -322,14 +322,14 @@
 
         if(task.Child is CohortAggregateContainer cac)
         {
-            foreach (var cacac in cac.GetAggregateConfigurations())
+            var offendingConfig = cac
+                .GetAggregateConfigurations()
+                .FirstOrDefault(ac => ac.Catalogue != null && ac.Catalogue.IsInternalDataset);
+
+            if (offendingConfig != null)
             {
-                if (cacac.Catalogue != null && cacac.Catalogue.IsInternalDataset)
-                {
-                    task.CrashMessage = new ArgumentException($"Catalogue {cacac.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations.");
-                    task.State = CompilationState.Crashed;
-                    break;
-                }
+                task.CrashMessage = new ArgumentException($"Catalogue {offendingConfig.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations.");
+                task.State = CompilationState.Crashed;
             }
         }
 
EOF
@@ -322,14 +322,14 @@

if(task.Child is CohortAggregateContainer cac)
{
foreach (var cacac in cac.GetAggregateConfigurations())
var offendingConfig = cac
.GetAggregateConfigurations()
.FirstOrDefault(ac => ac.Catalogue != null && ac.Catalogue.IsInternalDataset);

if (offendingConfig != null)
{
if (cacac.Catalogue != null && cacac.Catalogue.IsInternalDataset)
{
task.CrashMessage = new ArgumentException($"Catalogue {cacac.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations.");
task.State = CompilationState.Crashed;
break;
}
task.CrashMessage = new ArgumentException($"Catalogue {offendingConfig.Catalogue.Name} is marked as Internal. Internal Catalogues cannot be used in Cohort Identification Configurations.");
task.State = CompilationState.Crashed;
}
}

Copilot is powered by AI and may make mistakes. Always verify output.
@JFriel JFriel marked this pull request as ready for review February 9, 2026 14:13
@JFriel JFriel merged commit f61b66d into develop Feb 9, 2026
6 checks passed
@JFriel JFriel deleted the bugfix/RDMP-359-internal-catalogue-bugs branch February 9, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants